Deduplicated float tests and unified in floats/mod.rs #148206
+705
−1,345
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
Collaborator
|
|
This comment has been minimized.
This comment has been minimized.
bjorn3
reviewed
Oct 28, 2025
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Any update @bjorn3 ? |
Member
|
CI is still failing. |
Contributor
Author
ok let me fix . |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
xonx4l
force-pushed
the
deduplicate-float-tests
branch
from
b802b19 to
abd73fd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| let inf: Float = Float::INFINITY; | ||
| let neg_inf: Float = Float::NEG_INFINITY; | ||
| assert_biteq!((10.0 as Float).log(10.0), 1.0); | ||
| assert_approx_eq!((2.3 as Float).log(3.5), 0.664858, Float::LOG_APPROX); |
Contributor
There was a problem hiding this comment.
This test here for f128 will fail because the precision is much higher, if you look at the original test, the expected value is much more precise than what you have written:
assert_approx_eq!(2.3f128.log(3.5), 0.66485771361478710036766645911922010272, TOL);When testing higher precision floats, you need much more precise expected values or the tests will fail, as they do now. In Miri, this is even more strict.
If you look at the source of f16 constants, they use much more digits after the decimal point than needed:
pub const PI: f16 = 3.14159265358979323846264338327950288_f16;So you should probably do that as well.
Contributor
Author
There was a problem hiding this comment.
Okay it makes sense . Thanks
Contributor
|
Thank you for the final cleanup here. Until CI is fixed, |
rustbot
added
S-waiting-on-author
Collaborator
|
Reminder, once the PR becomes ready for a review, use |
Contributor
Author
Will finish this as soon as possible . A bit busy these days. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
xonx4l
force-pushed
the
deduplicate-float-tests
branch
from
0f27f23 to
507fb72
Compare
rust-bors bot
pushed a commit
that referenced
this pull request
Jan 31, 2026
Deduplicated float tests and unified in floats/mod.rs try-job: aarch64-apple try-job: x86_64-mingw-1
tgross35
reviewed
Jan 31, 2026
Comment on lines
189
to
195
| // FIXME(f128): f128 math operations need f128 math symbols, which currently aren't always | ||
| // filled in by compiler-builtins. The only libc that provides these currently is glibc. | ||
| let has_reliable_f128_math = has_reliable_f16_f128 && sess.target.env == Env::Gnu; | ||
|
|
||
| TargetConfig { | ||
| target_features, | ||
| unstable_target_features, |
This comment has been minimized.
This comment has been minimized.
Contributor
|
💔 Test for fc578bb failed: CI. Failed job:
|
Contributor
Author
|
In the logs it appears the MinGW math library has a bug in tgamma implementation. |
tgross35
reviewed
Jan 31, 2026
| assert!(Float::NAN.gamma().is_nan()); | ||
| assert!(Float::NEG_INFINITY.gamma().is_nan()); | ||
| assert_biteq!(Float::INFINITY.gamma(), Float::INFINITY); | ||
| assert_biteq!(flt(1760.9).gamma(), Float::INFINITY); |
Contributor
There was a problem hiding this comment.
The reason the failure started showing up is that this used to be 171.71 for all float types, and 1760.9 only for f128. You should change this to:
// FIXME: there is a bug in the MinGW gamma implementation that causes this to
// return NaN. https://sourceforge.net/p/mingw-w64/bugs/517/
if !cfg!(all(target_os = "windows", target_env = "gnu", not(target_abi = "llvm"))) {
assert_biteq!(flt(1760.9).gamma(), Float::INFINITY);
}
if Float::Int::BITS <= 64 {
assert_biteq!(flt(1760.9).gamma(), Float::INFINITY);
}
4 tasks
xonx4l
force-pushed
the
deduplicate-float-tests
branch
from
cae80de to
fa47b93
Compare
Contributor
|
Don't squash the cranelift patch, keep that separate. Having good notes in the blame and history is important. The rest can (an should) remain squashed, though. I made a typo in the snip I posted, it should be // FIXME: there is a bug in the MinGW gamma implementation that causes this to
// return NaN. https://sourceforge.net/p/mingw-w64/bugs/517/
if !cfg!(all(target_os = "windows", target_env = "gnu", not(target_abi = "llvm"))) {
assert_biteq!(flt(1760.9).gamma(), Float::INFINITY);
}
if Float::Int::BITS <= 64 {
assert_biteq!(flt(171.71).gamma(), Float::INFINITY);
}(second assert has 171.71 as the input, not the same thing twice) |
xonx4l
force-pushed
the
deduplicate-float-tests
branch
from
fa47b93 to
dc9db23
Compare
Contributor
Author
|
And I thought why not to just use && and make it a single check. That looked a bit weird I should have ask but no worries I will update it now . |
Contributor
Author
Just to be clear here u talking about the disable math tests patch or the patch that you made in cranelift lib.rs about not to squash? |
This comment has been minimized.
This comment has been minimized.
Contributor
The commit I pushed 451738f, since it's kind of separate from the rest here. I can help with the history if needed since git can be tricky. That's a rather annoying error. I guess maybe add |
tgross35
reviewed
Jan 31, 2026
Comment on lines
4
to
5
| #[doc(hidden)] | ||
| trait TestableFloat: Sized { |
Contributor
There was a problem hiding this comment.
#[doc(hidden)] shouldn't be needed, it's a private trait
xonx4l
force-pushed
the
deduplicate-float-tests
branch
from
dc9db23 to
92c9baf
Compare
Contributor
Author
Okay! adding a constant |
Contributor
|
@bors try jobs=aarch64-apple,x86_64-mingw-1 |
This comment has been minimized.
This comment has been minimized.
rust-bors bot
pushed a commit
that referenced
this pull request
Jan 31, 2026
Deduplicated float tests and unified in floats/mod.rs try-job: aarch64-apple try-job: x86_64-mingw-1
xonx4l
force-pushed
the
deduplicate-float-tests
branch
from
92c9baf to
063a9de
Compare
Contributor
Contributor
|
Try build cancelled. Cancelled workflows: |
This comment has been minimized.
This comment has been minimized.
rust-bors bot
pushed a commit
that referenced
this pull request
Jan 31, 2026
Deduplicated float tests and unified in floats/mod.rs try-job: aarch64-apple try-job: x86_64-mingw-1
xonx4l
force-pushed
the
deduplicate-float-tests
branch
from
063a9de to
eef1838
Compare
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
💔 Test for 7250dcf failed: CI. Failed job:
|
Contributor
Author
|
@tgross35 what you suggest for these macOS linker errors? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR Float tests are deduplicated and are unified in floats/mod.rs, as discussed in #141726.
The moved float tests are:
-> test_powf
-> test_exp
-> test_exp2
-> test_ln
-> test_log_generic
-> test_log2
-> test_log10
-> test_asinh
-> test_acosh
-> test_atanh
-> test_gamma
-> test_ln_gamma
Closes: #141726
r? tgross35